As a first look, I really dislike move all pin_arduino.* files.
As I expected the number of duplicated files/code are impressive and will growth with the new boards addition.
Maintains this would become hard.

Why not, for example add some guard:

#ifndef PIN_A0 
...
#endif

and define Pin_A0 in the variant.h.

or add a custom define :

#ifndef CUSTOM_PIN_ARDUINO
...
#endif

Anyway, as I said, it's a first feeling and I have to go further. 😉

I'm willing to do whatever it takes for this PR to be merged 🙂 I'm busy tonight, but will continue working on this tomorrow. I'll see what I can do about the global pins_arduino.c/h files.

But apart from this, what do you think? Positive?

I guess removing Analog pins restrictions is really fine but as said moving the pin_arduino files is not the good way, see the diff numbers:

+27,286
-0

while 99% are the same.
Anyway, it makes me think about a way to achieve this in a more generic way but I have to test it.

I'm absolutely open to other ways of achieving this.

However, I do need full control over :

• all the analog pin definitions
• all the PIN_SPI and PIN_WIRE macros
• the ATEMP, VREF and AVBAT macros
• the analogInputToPinName macro
• Get rid of some static asserts that prevents this from being compiled

• all the analog pin definitions

As I said could be achieved easily with guard

• all the PIN_SPI and PIN_WIRE macros

This is already the case

• the ATEMP, VREF and AVBAT macros

Why this is hardware specific and not linked to a pin and already managed.

• the analogInputToPinName macro

Same, with a guard or a define.

• Get rid of some static asserts that prevents this from being compiled

Static assert will be updated if needed and are present to avoid any issue with wrong definition.

the ATEMP, VREF and AVBAT macros

Why this is hardware-specific and not linked to a pin and already managed.

To prevent compiler warnings I had to change the numbers a bit on these macros. I defined ATEMP as NUM_DIGITAL_PINS + 2 instead of NUM_DIGITAL_PINS + 1, because +1 is already used in analogInputToDigitalPin. I didn't use NC in analogInputToDigitalPin because the compiler complained about comparing different types and enums.

I'll see if I can roll back the commit where all the pins_arduino.c/h files were added.

I didn't use NC in analogInputToDigitalPin because the compiler complained about comparing different types and enums.

Right and it is fine the compiler raise warning. This force to use cast if really needed.
Note that NC is used to know if the pin is correct or not so you probably introduce regression.

All you mentioned as a requirement is de facto linked how pins are managed. This simply means all have to be aligned with the new implementation. Which is normal. 😉

Right and it is fine the compiler raise warning. This force to use cast if really needed.
Note that NC is used to know if the pin is correct or not so you probably introduce regression.

Sorry, but I'm having difficulties understanding what you mean here. Compiler warnings are there for a reason; the code isn't written the way it should. I always tries my best to get rid of warnings, because it "pollutes" the compiler output. IMO the core files should not generate warnings at all. This way, if the users see some red text in the output window, it is not caused by the core files, but rather the user program or included libraries. What I can do however is to just return (p) if the pin isn't correct. this is what's originally done anyways, and does not generate any warnings. The analogInputToDigitalPin is rarely used by user programs anyways; It's mainly used within the core files.

All you mentioned as a requirement is de facto linked how pins are managed. This simply means all have to be aligned with the new implementation. Which is normal.

Hmm. I don't understand everything here, but I assume you think the requirements I have for this are OK. Am I right?

Right and it is fine the compiler raise warning. This force to use cast if really needed.
Note that NC is used to know if the pin is correct or not so you probably introduce regression.

Sorry, but I'm having difficulties understanding what you mean here. Compiler warnings are there for a reason; the code isn't written the way it should. I always tries my best to get rid of warnings, because it "pollutes" the compiler output. IMO the core files should not generate warnings at all. This way, if the users see some red text in the output window, it is not caused by the core files, but rather the user program or included libraries. What I can do however is to just return (p) if the pin isn't correct. this is what's originally done anyways, and does not generate any warnings. The analogInputToDigitalPin is rarely used by user programs anyways; It's mainly used within the core files.

Well forgot that and focus only on the original requirement: remove analog Pins definitions limitations

Hmm. I don't understand everything here, but I assume you think the requirements I have for this are OK. Am I right?

Yes and no. I just want to say that "your" requirements are all dependent and related to each other.
If you change the way analog are defined so you need to update all related stuff...
So given a full control on ATEMP, for example, is not (from my point of view) a requirement as it is not a "pin" but an internal ADC. It has nothing to do in the variant folder just have to be updated in a way it does not disturbed analog pins management.

@fpistm how about now? Now only minimal changes to the global pins_arduino.h file are done. It still works as it should, and does not affect any other variants.

@MCUdude
This is better than the previous version 😉
Less risky and easier to maintain, it focuses on what you wanted to do.

So what changes do I have to do in order tofix the Astyle formatting and hopefully get this merged?

See details of the check you will see what goes wrong.
You can use Astyle to format properly the PR.
See https://github.com/stm32duino/wiki/wiki/Astyle

About merge, I will have to review deeply and check all uses case.
And the commits needs to be reworked.
Adding a file in a commit then delete it in one other is not good practice.
Each commit have to be for one goal and should be build able --> atomic
See the CONTRIBUTING.md

And the commits needs to be reworked.

To be honest, I'm absolutely no git expert, at all, sorry about that. Would you like it to be one commit where I add the variant files, one commit where I add the new variant to the boards.txt and one commit where I change the global pins_arduino.h file? In that case I'll have to roll back everything and start clean.

You do not need to be a git expert.
I advise you to read the contributing.md, mainly this part:
https://github.com/stm32duino/Arduino_Core_STM32/blob/master/CONTRIBUTING.md#5-rebasing-pull-requests

Your PR should only contains 2 commits:

• 002dc26 should be the first with a commit message more detailed (why you do this?)
• all others should be squashed in 1 commit

Thanks for the heads-up, hopefully, it's OK now.

EDIT: No, I still had messed some formatting. It should be OK now.

Some new commits. I've tried to add descriptive comments.
The last commit is also properly tested on actual hardware. I've tried to remove (and short out) the external oscillator, and it switches to its internal oscillator on boot. The code you linked in for this was for an F103C8, but I found the correct code for it a little deeper into the repo.

Done! Anything else you'd like to be changed?

In fact, I will split this PR in 2 parts:

• Analog pins definition rework
• Add generic F401Rx variant

Analog pins definition rework

Can you point out to me what exact commits/commit hashes you'd want me to remove/roll back?

I would not have the variant addition in the same PR than the analog pin definitions rework.
Because I will probably do more update in the rework and also ease track for required update (ex: Firmata)

I would assume you'd mean all changes I've done to pins_arduino.h? Or just the AEND part of it?

The reason why I did it this way is that the generic F401Rx pinout relies on the changes done to pins_arduino.h.

I'm not trying to be rude or anything, but it has been pretty time consuming for me with everything that I've put into this PR. It would be great if you were more specific about what you want and what you don't want to be included when I first opened this PR. Anyways, I'm slowly learning from my mistakes, and hopefully, next time can just provide a clean PR without having to re-do commits again and again. Should be better for us both 😉

@fpistm would be great if you could reply to my previous post. Hopefully, I can finish this up today 👍

@MCUdude
Sorry, I have a huge workload.
Well to answer you, it's also pretty time consuming for me. I've spent several time also to answer you and give advice.
If we get back 5 days ago your PR made a lot of useless changes

+27,286
-0

It is now better formed. 😉

All what I advised are described in the Contributing.md (best practice, commits rules, ...).
As an example, it still a commit for removing the MassStorage upload method. Well, why this commit? This PR add and remove it, so this should not be there. Again simply squash this commit to the one which add it then this will clean the PR instead of having annoying codes changes.
Maybe, I'm not enough clear and concise and I'm sorry about that. I do my best to answer and support all request and ensure a clean git history. 😞 😞

Currently, my concerns is that the PR should be ONLY to add the generic F401Rx variant.
I will open a new PR based on your work around the analog pins definition (which still need several tests to ensure not add regression) and then I will be able to merge this PR to add the generic board support.

So give me some time to do the job. 😉

This PR now only contains the generic variant, and not anything else. Do you want me to open a new PR where changes to pins_arduino.h are done or do you want to do this yourself to get it exactly how you want it to be?

Thanks @MCUdude
I will do the PR as I will probably add some more related commits.

Thanks @MCUdude
I will do the PR as I will probably add some more related commits.

Great! I'm pretty sure Travis CI fails to compile this PR because it's missing the PR you're about to do that changed the behavior of pins_arduino.h. But apart from this, are you satisfied with the PR now? I assume you won't (and can't) merge this before pins_arduino.h is modified.

At a first look, it seems OK but I will deeply review and test it later using the new PR I will submit.
And yes this will not be merged before the new one.

Required #800

@MCUdude
unfortunately, as I expected redefining analogInputToDigitalPin is not the good way.
In this variant it works but for other this could not because there is no way to have the same value for
AnalogRead(A0) and analogRead(0).

Ex of pin definitions:

#define PC13 0
#define PC14 1
#define PC15 2
#define PF0  3
#define PF1  4
#define PC0  5  // A0
#define PC1  6  // A1
#define PC2  7  // A2
#define PC3  8  // A3
#define PA0  9  // A4/USER_BTN
#define PA1  10 // A5
#define PA2  11 // A6
#define PA3  12 // A7
#define PF4  13
#define PF5  14
#define PA4  15 // A8
#define PA5  16 // A9
#define PA6  17 // A10
#define PA7  18 // A11
#define PC4  19 // A12
#define PC5  20 // A13
#define PB0  21 // A14
#define PB1  22 // A15

Well, the pinout has to be mapped in a specific way to support both analogRead(0) and analogRead(A0). That's just the nature of it. TBH a function like analogReadChannel(0) would be much more suitable, because it may be difficult to combine the Ax constant and the channel number

What pinout is the code posted above from?

I suggest that all analog pins are only available on pinouts that allow channel number and pin number to be combined, like in this pinout

What pinout is the code posted above from?

Disco_F030R8

I suggest that all analog pins are only available on pinouts that allow channel number and pin number to be combined, like in this pinout

Unfortunately, this will not be maintainable and hard to review. This have to be as generic as possible.

It seems like for the Disco F030R8 the NUM_ANALOG_FIRST is defined as 50. This makes for a nice offset to prevent channel numbers and Ax constants to overlap. However, you can't expect analogRead(5) (or analogRead(PC0)) to work, because it will choose analog channel 5, not the channel attached to digital pin 5.

The question really is, How can we make all analog pins available on as many targets as possible. To me, it's not as important if the what number/constant/macro the analogRead function takes, but it has to be consistent. What do you suggest? How can we achieve 16 analog inputs on the generic F401Rx pinout without having to re-map the digital pins to fit/match the analog ones?

In your proposal, NUM_ANALOG_FIRST is not used so this could not work.
We agreed this need to be consistent that's why I had split your PR, to have a dedicated one focused on analog pin definition.
I have an idea but I need some time to work on it.

Looking forward to seeing what you're able to come up with. What's important for me is that we can keep this exact pin mapping and still be able to access all analog pins in some way. This should be a goal for this repository as well IMO 🙂

Looking forward to seeing what you're able to come up with. What's important for me is that we can keep this exact pin mapping and still be able to access all analog pins in some way. This should be a goal for this repository as well IMO 🙂

Unfortunately, I will not be able to find a way to support this.
This is always the same drawback having:
analogRead(Ax) == analogRead(x) == analogRead(PYn)

If PYn is less than NUM_ANALOG_INPUTS, there is no way to differentiate if x or PYn.

So if you want the generic variant for F401Rx, you will have to reorder the pin number.

//                 | ANALOG | USART     | TWI      | SPI                    | SPECIAL   |
//                 |--------|-----------|----------|------------------------|-----------|
#define PA0  21 // | A0     |           |          |                        |           |
#define PA1  22 // | A1     |           |          |                        |           |
#define PA2  23 // | A2     | USART2_TX |          |                        |           |
#define PA3  24 // | A3     | USART2_RX |          |                        |           |
#define PA4  25 // | A4     |           |          | SPI1_SS, (SPI3_SS)     |           |
#define PA5  26 // | A5     |           |          | SPI1_SCK               |           |
#define PA6  27 // | A6     |           |          | SPI1_MISO              |           |
#define PA7  28 // | A7     |           |          | SPI1_MOSI              |           |
#define PA8  0  // |        |           | TWI3_SCL |                        |           |
#define PA9  1  // |        | USART1_TX |          |                        |           |
#define PA10 2  // |        | USART1_RX |          |                        |           |
#define PA11 3  // |        | USART6_TX |          |                        |           |
#define PA12 4  // |        | USART6_RX |          |                        |           |
#define PA13 5  // |        |           |          |                        | SWD_SWDIO |
#define PA14 6  // |        |           |          |                        | SWD_SWCLK |
#define PA15 7  // |        |           |          | SPI1_SS, (SPI3_SS)     |           |
//                 |--------|-----------|----------|------------------------|-----------|
#define PB0  29 // | A8     |           |          |                        |           |
#define PB1  30 // | A9     |           |          |                        |           |
#define PB2  8 // |        |           |          |                        | BOOT1     |
#define PB3  9 // |        |           | TWI2_SDA | SPI1_SCK,  (SPI3_SCK)  |           |
#define PB4  10 // |        |           | TWI3_SDA | SPI1_MISO, (SPI3_MISO) |           |
#define PB5  11 // |        |           |          | SPI1_MOSI, (SPI3_MOSI) |           |
#define PB6  12 // |        | USART1_TX | TWI1_SCL |                        |           |
#define PB7  13 // |        | USART1_RX | TWI1_SDA |                        |           |
#define PB8  14 // |        |           | TWI1_SCL |                        |           |
#define PB9  15 // |        |           | TWI1_SDA | SPI2_SS                |           |
#define PB10 16 // |        |           | TWI2_SCL | SPI2_SCK               |           |
#define PB12 17 // |        |           |          | SPI2_SS                |           |
#define PB13 18 // |        |           |          | SPI2_SCK               |           |
#define PB14 19 // |        |           |          | SPI2_MISO              |           |
#define PB15 20 // |        |           |          | SPI2_MOSI              |           |
//                 |--------|-----------|----------|------------------------|-----------|
#define PC0  31 // | A10    |           |          |                        |           |
#define PC1  32 // | A11    |           |          |                        |           |
#define PC2  33 // | A12    |           |          | SPI2_MISO              |           |
#define PC3  34 // | A13    |           |          | SPI2_MOSI              |           |
#define PC4  35 // | A14    |           |          |                        |           |
#define PC5  36 // | A15    |           |          |                        |           |
#define PC6  37 // |        | USART6_TX |          |                        |           |
#define PC7  38 // |        | USART6_RX |          |                        |           |
#define PC8  39 // |        |           |          |                        |           |
#define PC9  40 // |        |           | TWI3_SDA |                        |           |
#define PC10 41 // |        |           |          | SPI3_SCK               |           |
#define PC11 42 // |        |           |          | SPI3_MISO              |           |
#define PC12 43 // |        |           |          | SPI3_MOSI              |           |
#define PC13 44 // |        |           |          |                        |           |
#define PC14 45 // |        |           |          |                        | OSC32_IN  |
#define PC15 46 // |        |           |          |                        | OSC32_OUT |
//                 |--------|-----------|----------|------------------------|-----------|
#define PD2  47 // |        |           |          |                        |           |
//                 |--------|-----------|----------|------------------------|-----------|
#define PH0  48 // |        |           |          |                        | OSC_IN    |
#define PH1  49 // |        |           |          |                        | OSC_OUT   |
//                 |--------|-----------|----------|------------------------|-----------|

As for generic variant, user mainly use the PYn notation this will not be an issue.
Of course, the digitalPin[] array have to be reordered and define NUM_ANALOG_FIRST to 21.

Unfortunately, I will not be able to find a way to support this.
This is always the same drawback having:
analogRead(Ax) == analogRead(x) == analogRead(PYn)
If PYn is less than NUM_ANALOG_INPUTS, there is no way to differentiate if x or PYn.

How is this solved on the Disco_F030R8 today? I bet analogRead(Ax) == analogRead(x) == analogRead(PYn) wont work. Any my pinout has nothing to do with this.

Why would it be a problem to modify the analogInputToDigitalPin just for this pinout? It allows for flexible analogRead, and it does not affect any other pinouts. All existing ones can stay the same.

How is this solved on the Disco_F030R8 today? I bet analogRead(Ax) == analogRead(x) == analogRead(PYn) wont work. Any my pinout has nothing to do with this.

Currently, this works.

Why would it be a problem to modify the analogInputToDigitalPin just for this pinout? It allows for flexible analogRead, and it does not affect any other pinouts. All existing ones can stay the same.

Because, it could not be applied to all possible pins mapping.

Anyway, I've just found a way to have a new generic way to define analog pins and seems removed all drawback.

Why would it be a problem to modify the analogInputToDigitalPin just for this pinout? It allows for flexible analogRead, and it does not affect any other pinouts. All existing ones can stay the same.

Because, it could not be applied to all possible pins mapping.

I don't really see why analogInputToDigitalPin has to be the same on all pinouts. After all, this is a helper macro that is used to get the "Arduino things" right.

Anyway, I've just found a way to have a new generic way to define analog pins and seems removed all drawback.

Tell me about it!

Bottom line: what I eventually want to achieve with this pinout (and maybe more in the future) is that a user can Start Arduino IDE or VSCode/PlatformIO and just pick the STM32 that's needed for the job. No board, NO only the top of the line chip available, but all memory options. A little bit like what I've done with my various AVR cores. This is also super useful if you find a commercial product with an STM32 on that you's like to write your own FW to.

Bottom line: what I eventually want to achieve with this pinout (and maybe more in the future) is that a user can Start Arduino IDE or VSCode/PlatformIO and just pick the STM32 that's needed for the job. No board, NO only the top of the line chip available, but all memory options. A little bit like what I've done with my various AVR cores. This is also super useful if you find a commercial product with an STM32 on that you's like to write your own FW to.

I think you don't understand you can already do that... As I said for generic board user will use the PYn format and are not aware of which value it is in the background. So, I really do not understand your point about that as the pin mapping could easily be done like I do in a previous comment. While doing an analogread(PYn) works...

@MCUdude

Tell me about it!

this is now available in #800

OK. Fixed now

How about now?

@MCUdude is it possible for you to create also generic variant for F407VET/VGT?

@MCUdude is it possible for you to create also generic variant for F407VET/VGT?

It's certainly is possible, but I'm not planning to use an F407 in a design at the moment. I also don't have any F407 hardware to test with. However, I do plan to add F401Cx, F411Rx, and F411Cx.